fix(proxyabl): declare jose as a runtime dependency#35
Open
davidcrowe wants to merge 1 commit into
Open
Conversation
@gatewaystack/proxyabl-core imports jose in auth.ts and oidc.ts but never declared it, so a fresh npm install of the published package failed on first import with ERR_MODULE_NOT_FOUND — it only worked inside this monorepo, where jose is hoisted from identifiabl-core. Pin ^6.1.0 to match identifiabl-core, bump core to 0.1.1 and middleware to 0.0.13, and give the middleware a jose devDependency for its type-only import so it typechecks outside the workspace.
This was referenced Jul 23, 2026
Collaborator
Author
|
Reviewed + confirmed green (build + test SUCCESS, MERGEABLE/CLEAN). Verified the jose classification is correct: @davidcrowe — needs your merge + republish (OTP): |
davidcrowe
added a commit
that referenced
this pull request
Jul 24, 2026
/#37/#47) (#50) * transformabl-core: expand PiiType union + regulatory mapping Prepares for Presidio-parity and HIPAA-relevant recognizers by widening the PiiType union and adding regulatory-category mappings for the new types. Patterns themselves land in the next commit. New types: - US identifiers: us_bank_number, us_itin, us_passport, us_drivers_license - International banking: iban - Healthcare (HIPAA PHI): icd_10, icd_9, npi - Financial: crypto_wallet Regulatory mapping adds PCI (bank accounts, IBAN) and HIPAA (all three healthcare types) attribution so classify.ts can report compliance categories in ContentMetadata. * transformabl-core: add Presidio-parity + healthcare PII recognizers Extends detectPii with recognizers that Microsoft Presidio ships by default (US-focused) plus healthcare-specific additions that are HIPAA-standard but not covered by Presidio's core list. Added: - us_bank_number: 8-17 digits (loose by design) - us_itin: 9XX-[78]X-XXXX — more specific than SSN pattern - us_passport: 9 digits, or 1 letter + 8 digits - us_drivers_license: letter + 7-8 digits, or prefixed form - iban: ISO 13616 (2 letters + 2 digits + 11-30 alphanum) - icd_10: letter + 2 chars + optional .Xn (up to 4 decimals) - icd_9: 3 digits + optional .XX, or V/E codes - npi: 10-digit starting with 1 or 2 - crypto_wallet: BTC (P2PKH + bech32) + ETH (0x + 40 hex) Extended: - phone: now matches E.164 international (+<country><digits>) - ip_address: now matches IPv6 full and compressed forms Motivation: ACP's content governance layer silently passed through bank account numbers and ICD diagnosis codes (surfaced by an external multi-agent privacy benchmark, AgentLeak). These are standard PII categories in financial and healthcare contexts — a governance layer that misses them can't credibly claim PII coverage. Coverage surfaces via existing detectPii() API; no consumer changes required beyond bumping the dep pin. 26 new unit tests; all 68 tests pass. * transformabl-core: bump to 0.4.0 Minor version because the PiiType union is additive (no breaking changes for existing consumers) but the set of types returned by detectPii() has grown — callers that hard-code a closed set of expected types should review. * fix(transformabl-core): ReDoS in email PII regex + scan-length cap (0.4.1) Unbounded quantifiers around a single mandatory @ made the email pattern O(n^2): a long alphanumeric run (no @ needed) forced scan-to-end-then- backtrack from every start position — ~1MB blocked the event loop minutes. Bound to RFC 5321 limits (local<=64, domain<=255, tld<=24) + a 512KB detectPii scan cap. Verified: 300k-char attack 4413ms -> 39ms, emails still detected. Security review 2026-07-01. * fix(proxyabl): declare jose as a runtime dependency @gatewaystack/proxyabl-core imports jose in auth.ts and oidc.ts but never declared it, so a fresh npm install of the published package failed on first import with ERR_MODULE_NOT_FOUND — it only worked inside this monorepo, where jose is hoisted from identifiabl-core. Pin ^6.1.0 to match identifiabl-core, bump core to 0.1.1 and middleware to 0.0.13, and give the middleware a jose devDependency for its type-only import so it typechecks outside the workspace. * fix(explicabl): drop undeclared node-fetch import; use Node 18+ global fetch health.ts imported `node-fetch` but the package declared only express and express-rate-limit. Because dist/index.js re-exports health.js, the published @gatewaystack/[email protected] threw ERR_MODULE_NOT_FOUND on ANY import from a fresh install — including createConsoleLogger, which has nothing to do with the health route. Same defect class as the proxyabl-core/jose break (PR #35). The two call sites use only standard WHATWG fetch semantics (.ok/.status, {method,headers,body}), so Node 18+'s global fetch is a drop-in. Removing the import also makes the README's 'zero dependencies beyond Express' claim true. Add engines.node >=18 to make the global-fetch requirement explicit; bump to 0.0.9. Verified: packed tarball installs into an empty project and imports with all 5 exports; full suite (159 tests) green. * docs(identifiabl): fix Quick Start to use the real export name The README's Basic Usage and later examples imported and called `createIdentifiablMiddleware`, which the package does not export — the Express middleware is `identifiabl(config)` (createIdentifiablVerifier is the non-Express verifier). Copy-pasting the Quick Start failed at the import line. Rename all five occurrences to the real export; config shape and req.user usage already match identifiabl()'s behavior, so no other changes needed. * Remove hardcoded conformance badge, report, and CI artifact (#39) saveReport.ts wrote all five conformance categories (pkce, jwt_verify, scope_enforcement, allowlist, expiry) as "pass" with a fresh timestamp, consulting no test — and npm test ran it after every suite. The committed docs/conformance.json reflected that, and the README badge read its version. For a repo-visibility push this is the single highest-risk artifact: one file-read of saveReport.ts discredits every other claim. Rather than fake a conformance suite, remove it. PKCE is not implemented in this repo (the OAuth/PKCE flow is delegated to Auth0; the only "pkce" reference is a ChatGPT-DCR detection heuristic), so that category cannot be honestly asserted at all. The other four map to real unit-tested code, but badging library unit tests as "OAuth 2.1 + MCP Authorization conformance" overclaims the same way. An absent badge is honest; a hardcoded one is not. A real MCP Authorization conformance suite is a separate, larger effort. Removed: - README "MCP/Auth Conformance" badge - docs/conformance.json - packages/explicabl-core/src/reporting/saveReport.ts (+ compiled .js) - docs/testing/conformance.md (empty) - .github/workflows/conformance.yml (redundant: build.yml already runs tests; its only distinct job was uploading conformance.json) - the conformance:report npm script; stripped it from the test script - conformance mentions in operations/testing/deployment docs + CONTRIBUTING Build green, 159 tests pass, npm test no longer regenerates the file. * transformabl-core 0.4.1: redactPii overlap fix (H4) + scan-truncation flag (M2) Builds on the ReDoS fix in #30 (this branch's base). Adds the two remaining 0.4.1 items from #40 and bumps the facade so consumers can receive it. H4 — redactPii overlap corruption (src/redact.ts): Matches were substituted by descending start with each match's own stale `end`. When an inner overlapping match was replaced first, it shifted the text so an outer match's `end` sliced the wrong point, leaving trailing PII digits behind in placeholder/remove modes. #30's new recognizers make overlaps routine (us_bank_number's \d{8,17} covers the same run as npi / us_drivers_license). Fix: filter to the requested types, merge overlapping matches into maximal intervals, then substitute the disjoint spans right-to-left so no substitution can invalidate another's offsets. The merged span is masked/removed/labelled as a unit (placeholder uses the widest contributing type). match.value always equals text.slice(start,end), so non-overlapping behavior is unchanged. M2 — silent PII-scan truncation (src/detect.ts): #30 capped scanning at 512 KB with no signal, so PII past the cap flowed through unredacted and unlogged — fail-open that wasn't loud. Add detectPiiDetailed() returning { matches, scanTruncated, scannedLength, totalLength } and a scanTruncated flag on TransformResult; detectPii stays a thin back-compatible wrapper. Exported MAX_PII_SCAN_LENGTH. Fixed the now-false normalize.ts comment (detection no longer scans the full input). Facade: bump @gatewaystack/transformabl-core range ^0.3.0 -> ^0.4.1 (^0.3.0 can never resolve 0.4.x) and the facade to 0.3.0 (re-exports detectPiiDetailed + MAX_PII_SCAN_LENGTH). Republish needed after publishing core 0.4.1. Tests: redact.overlap.test.ts (partial/identical/contained overlaps, types filter, no-surviving-substring acceptance) and scanTruncation.test.ts (flag on/off, PII before vs past the cap, transform surfaces it). Full suite 198 green. * Package hygiene: per-package LICENSE, engines, tsbuildinfo out of dist, dep fixes (#42) Class-level packaging defects from the 2026-07-23 sweep, across the 13 published packages. - **LICENSE shipped in zero tarballs.** Every package.json lists "LICENSE" in `files`, but the file existed only at repo root — npm only auto-includes LICENSE from the package dir, so every tarball claimed MIT and shipped no license. Added a per-package LICENSE (copy of root) and ensured "LICENSE" (and README.md where present) is in each `files` list. `npm pack --dry-run` now shows LICENSE in the tarball. - **tsconfig.tsbuildinfo shipped in dist** of the 10 packages that set `tsBuildInfoFile: dist/...` — a dead dev file in every tarball. Moved it to the package root (`tsconfig.tsbuildinfo`), which is outside the `files` whitelist, so it no longer ships. Verified no *.tsbuildinfo remains in any dist. (Already covered by .gitignore, so none get tracked.) - **No `engines` field anywhere** despite ESM-only + global-fetch reliance. Added `"node": ">=18"` to all published packages. - **request-context / observability-core had no prepublishOnly build** — stale-dist publish risk. Added `prepublishOnly: npm run build` wherever a build script exists but the hook was missing. - **proxyabl pinned request-context exactly** (`0.0.6`) while identifiabl uses `^` — a future request-context patch would force a proxyabl republish. Loosened to `^0.0.6`. - **identifiabl declared unused `jose@^5`** — its source/dist never import jose (only a stale doc comment referenced it), and it caused a dual-jose install (5.x hoisted + 6.x under core). Dropped it. Build clean, full suite 159 green. Out of scope here (flagged): the transformabl facade range ^0.3.0 -> ^0.4.1 is handled in the 0.4.1 release PR (#45). The four declared-but-unused deps in the "badge-padding" item live in gatewaystack-connect (the prod repo), not here. Also noted: packages/explicabl-core/ contains the package NAMED @gatewaystack/observability-core — a dir/name mismatch worth a follow-up. * Correct proxyabl SSRF marketing to match what actually runs (#43) proxyabl-core's assertUrlSafe/executeProxyRequest (DNS-resolving private-IP blocking, host allowlist, redirect:"manual", size/timeout caps) are exported and unit-tested but have zero consumers outside their own tests. The README and docs implied the gateway proxy runs this engine; it does not. Decision: correct the marketing (the issue's "Or" option), not wire the engine into the router — because the router's model is genuinely different, not a bug to paper over: - The three user-facing router fetches (mcpHandler, restToolHandler, proxyHandler) all forward to an operator-CONFIGURED backend (functionsBase / proxy target). The caller controls the tool name / path, not the host, which is pinned to the configured backend via a host+protocol equality check. - executeProxyRequest is built for the opposite case — proxying arbitrary, caller-influenced URLs against an allowlist. Dropping it into the router would (a) reject configured backends that are legitimately internal (private-IP blocking), (b) change response semantics (it buffers+caps and throws on non-2xx instead of passing upstream status/body through), and (c) block upstream redirects the router may rely on. That is a breaking change to live gateway behavior to satisfy a marketing line — backwards. - OSS has no arbitrary-URL / custom-connector path today (executeProxyRequest's ancestor served custom HTTP tools that live only in prod), so the engine has no natural OSS consumer yet. Changes (docs only): - README module table: proxyabl row now describes the router accurately (configured-backend, path-allowlisted, identity-aware) and points to the proxyabl-core SSRF primitive. - README: added an "SSRF note" spelling out the two threat models. - docs/packages.md: split the proxyabl entry into the core SSRF-safe primitive vs the pass-through Express router, and noted the router does not route through the engine + the convergence direction. - proxyabl-core/README: added a scope note so a reader (or a reviewer grepping assertUrlSafe) sees the primitive is intentional and standalone, not dead. The proxyabl-core README's own SSRF description was already accurate for the primitive and is unchanged except for the note. The real fix — one canonical proxyabl-core engine consumed by both the OSS router and prod's fork — is the convergence epic; flagged, not attempted here. * Add a zero-config quickstart example: governance in 30 seconds The on-ramp led with `identifiabl` (JWT), which 401s every request until you stand up an IdP — so a new developer's first run showed nothing working. This adds a runnable, zero-config first win. `examples/quickstart` is a ~50-line script over the pure `-core` packages that prints three real decisions with no IdP, backend, or config: - deny-by-default policy (validatabl) — read_file allowed, delete_database denied - PII redaction (transformabl) — email + SSN → [EMAIL]/[SSN] - rate limit (limitabl) — the 4th call in the window is refused It uses only currently-published package versions, so `npm install && npm start` works today. README now offers this as the "see it work first" path before the IdP-backed middleware setup. CLAUDE.md's stale "tests are not yet implemented" status is corrected (suites exist under __tests__/). Deliberately does NOT demo the "rm -rf hardline" — that classifier lives in the private prod engine, not OSS; showing it here would claim a capability the repo doesn't ship. Verified: `npm run build` + the example runs and prints the decisions. * chore: regenerate lockfile after integrating cleanup PRs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
@gatewaystack/[email protected]importsjose(src/auth.ts:2,src/oidc.ts:7) but declares no dependencies at all, so the published package is broken on fresh install:It only works inside this monorepo because
joseis hoisted fromidentifiabl-core.@gatewaystack/proxyablinherits the breakage through its dependency on core.Fix
proxyabl-core: addjose@^6.1.0todependencies(matchesidentifiabl-core), bump to 0.1.1proxyabl: bump core constraint to^0.1.1, addjosetodevDependencies(its import is type-only — erased at runtime, needed for standalone typechecking), bump to 0.0.13Verification
npm packthe fixed core, install the tarball in an empty directory, import — succeeds with 17 exports (josepulled from the registry as a declared dep)Both packages need a republish after merge (
prepublishOnlyhandles the build).